[Security Solution] Remove a data fetching hook from the add to timeline action component #124331

kqualters-elastic · 2022-02-02T10:18:23Z

Summary

Fixes #124307 for all alert types by removing a hook that conditionally fetched data in every row. Instead this logic is moved into the callback from the click, so the data is fetched once and only once and only when needed. This api call can and should be removed soon by updating the alerts table search strategy to return all needed fields for building the timeline filters/data providers etc. By adding all kibana.alert.* referenced in https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx to https://github.com/elastic/kibana/blob/main/x-pack/plugins/timelines/server/search_strategy/timeline/factory/helpers/constants.ts#L45 the request that was previously made in useFetchEcsAlertsData and moved to https://github.com/elastic/kibana/compare/main...kqualters-elastic:fetch-ecs-in-actions?expand=1#diff-972b23879f146e612234cef06ac25c143709767be35c69d621861d7b50d8ce24R402 can be removed entirely.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

kqualters-elastic · 2022-02-02T14:19:30Z

/oblt-deploy

kqualters-elastic · 2022-02-02T14:52:22Z

/oblt-deploy

michaelolo24 · 2022-02-02T15:12:36Z

x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx

+      estypes.SearchResponse<{ '@timestamp': string; [key: string]: unknown }>
+    >(DETECTION_ENGINE_QUERY_SIGNALS_URL, {
+      method: 'POST',
+      body: JSON.stringify(buildAlertsQuery([ecsData._id] ?? [])),


is the nullish operator here necessary? The empty array will never be selected

ya it's not, will remove.

…tions

andrew-goldstein · 2022-02-02T18:36:05Z

x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx

@@ -550,7 +613,7 @@ export const sendAlertToTimelineAction = async ({
      });
    }
  } else if (isThresholdRule(ecsData)) {
-    createThresholdTimeline(ecsData, createTimeline, noteContent, {});
+    return createThresholdTimeline(ecsData, createTimeline, noteContent, {});


Is an await required in this context, similar to the return await createThresholdTimeline(ecsData, createTimeline, noteContent, { above?

I think it's the other way actually, the await is redundant and not needed above. Not sure why the linter did not catch this one, it catches the other

andrew-goldstein · 2022-02-02T19:17:31Z

x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx

+    });
+    const from = DEFAULT_FROM_MOMENT.toISOString();
+    const to = DEFAULT_TO_MOMENT.toISOString();
+    return createTimeline({


consider not invoking createTimeline here, because in this state, we can't fully construct the search criteria

andrew-goldstein

Thanks for this fix @kqualters-elastic! 🙏
Desk tested locally with threshold, eql, and other rule types
LGTM

...n/public/detections/components/alerts_table/timeline_actions/use_investigate_in_timeline.tsx

YulNaumenko · 2022-02-03T01:34:45Z

...ublic/detections/components/alerts_table/timeline_actions/investigate_in_timeline_action.tsx

@@ -22,7 +22,6 @@ interface InvestigateInTimelineActionProps {
  alertIds?: string[];


We need to do a proper clean up of alertIds usage. That would be great to clarify in the PR description why we don't need any more fetching the alerts and alertIds.

YulNaumenko · 2022-02-03T05:09:10Z

Thank you for fixing this!
Please, add the minimal testing coverage where it is possible. I think it could be some tests for x-pack/plugins/security_solution/public/detections/components/alerts_table/timeline_actions/use_investigate_in_timeline.tsx,
x-pack/plugins/security_solution/public/detections/components/alerts_table/timeline_actions/investigate_in_timeline_action.tsx, x-pack/plugins/security_solution/public/timelines/components/side_panel/event_details/footer.tsx

cnasikas

Cases changes LGTM

…tions

YulNaumenko

LGTM on green CI. Great work, thank you! It would be awesome to add the short notice about removing renderInvestigateInTimelineActionComponent in the PR description, just for improvement of maintaining history.

…tions

kibana-ci · 2022-02-08T06:32:42Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2871	2869	-2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`cases`	294.9KB	294.8KB	-103.0B
`securitySolution`	4.7MB	4.7MB	-1.1KB
`timelines`	226.6KB	226.5KB	-147.0B
total			-1.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`securitySolution`	246.3KB	246.4KB	+32.0B

History

💔 Build #22340 failed 474681b
💔 Build #22311 failed 407e506
💔 Build #22298 failed 39805d0
💛 Build #21887 was flaky 3250984
💚 Build #21412 succeeded 1f5c879
💔 Build #21292 failed c9d06f9

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2022-02-08T11:13:34Z

The following labels were identified as gaps in your version labels and will be added automatically:

v8.2.0

If any of these should not be on your pull request, please manually remove them.

kibanamachine · 2022-02-08T11:16:15Z

💔 All backports failed

Status	Branch	Result
❌	8.1	Backport failed because of merge conflicts You might need to backport the following PRs to 8.1: - Reverts PR 124310 (#124736)
❌	8.0	Backport failed because of merge conflicts You might need to backport the following PRs to 8.0: - [SECURITY] Introduce kibana nav (#68862)

How to fix

Re-run the backport manually:

node scripts/backport --pr 124331

Questions ?

Please refer to the Backport tool documentation

…ine action component (elastic#124331) * Fetch alert ecs data in actions.tsx and not a hook in every table row * Add error handling and tests for theshold timelines * Fix bad merge * Remove unused imports * Actually remove unused file * Remove usage of alertIds and dead code from cases * Add basic sanity tests that ensure no extra network calls are being made * Remove unused operator * Remove unused imports * Remove unused mock (cherry picked from commit e312c36)

kibanamachine · 2022-02-09T11:16:18Z